Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Synchronize files after writing #10735

Merged
merged 1 commit into from
May 13, 2024
Merged

Synchronize files after writing #10735

merged 1 commit into from
May 13, 2024

Conversation

the-mikedavis
Copy link
Member

fsync(2) is a somewhat expensive operation that flushes writes to the underlying disk/SSD. It's typically used by databases to ensure that writes survive very hard failure scenarios like your cat kicking the plug out of the wall. Synchronizing isn't automatically done by flushing (from the std::io::Write or tokio::io::AsyncWriteExt traits). From the tokio::fs::File moduledocs:

To ensure that a file is closed immediately when it is dropped, you should call flush before dropping it. Note that this does not ensure that the file has been fully written to disk; the operating system might keep the changes around in an in-memory buffer. See the sync_all method for telling the OS to write the data to disk.

This seems like a likely fix for #7618 (comment).

fsync(2) is a somewhat expensive operation that flushes writes to the
underlying disk/SSD. It's typically used by databases to ensure that
writes survive very hard failure scenarios like your cat kicking the
plug out of the wall. Synchronizing isn't automatically done by
`flush`ing (from the `std::io::Write` or `tokio::io::AsyncWriteExt`
traits). From the [`tokio::fs::File`] moduledocs:

> To ensure that a file is closed immediately when it is dropped, you
> should call `flush` before dropping it. Note that this does not ensure
> that the file has been fully written to disk; the operating system
> might keep the changes around in an in-memory buffer. See the
> `sync_all` method for telling the OS to write the data to disk.

[`tokio::fs::File`]: https://docs.rs/tokio/latest/tokio/fs/struct.File.html
@the-mikedavis the-mikedavis added this to the 24.05 milestone May 12, 2024
@kirawi
Copy link
Member

kirawi commented May 12, 2024

This looks to be what Neovim does too, so it was an oversight on my part: https://github.com/neovim/neovim/blob/4e5c633ed4871a948aff7338b793ac5f93484153/src/nvim/bufwrite.c#L1619-L1634

@kirawi
Copy link
Member

kirawi commented May 12, 2024

I think we should be calling fsync after restoring the backup since that's where it's called in the linked code.

@EriKWDev
Copy link

I'll be trying this fix. My computers crash or run out of battery pretty often sadly xP

@the-mikedavis
Copy link
Member Author

@kirawi where do you mean? I'm not familiar with the neovim codebase but it looks to me like they're calling fsync after writing and before performing any renaming or permissions/metadata changes

@kirawi
Copy link
Member

kirawi commented May 13, 2024

Oh yeah, you're right.

@archseer archseer merged commit 855568f into master May 13, 2024
6 checks passed
@archseer archseer deleted the write-fsync branch May 13, 2024 23:37
Vulpesx pushed a commit to Vulpesx/helix that referenced this pull request Jun 7, 2024
fsync(2) is a somewhat expensive operation that flushes writes to the
underlying disk/SSD. It's typically used by databases to ensure that
writes survive very hard failure scenarios like your cat kicking the
plug out of the wall. Synchronizing isn't automatically done by
`flush`ing (from the `std::io::Write` or `tokio::io::AsyncWriteExt`
traits). From the [`tokio::fs::File`] moduledocs:

> To ensure that a file is closed immediately when it is dropped, you
> should call `flush` before dropping it. Note that this does not ensure
> that the file has been fully written to disk; the operating system
> might keep the changes around in an in-memory buffer. See the
> `sync_all` method for telling the OS to write the data to disk.

[`tokio::fs::File`]: https://docs.rs/tokio/latest/tokio/fs/struct.File.html
kirawi pushed a commit to kirawi/helix that referenced this pull request Jun 9, 2024
fsync(2) is a somewhat expensive operation that flushes writes to the
underlying disk/SSD. It's typically used by databases to ensure that
writes survive very hard failure scenarios like your cat kicking the
plug out of the wall. Synchronizing isn't automatically done by
`flush`ing (from the `std::io::Write` or `tokio::io::AsyncWriteExt`
traits). From the [`tokio::fs::File`] moduledocs:

> To ensure that a file is closed immediately when it is dropped, you
> should call `flush` before dropping it. Note that this does not ensure
> that the file has been fully written to disk; the operating system
> might keep the changes around in an in-memory buffer. See the
> `sync_all` method for telling the OS to write the data to disk.

[`tokio::fs::File`]: https://docs.rs/tokio/latest/tokio/fs/struct.File.html
Chirikumbrah pushed a commit to Chirikumbrah/helix that referenced this pull request Jun 15, 2024
fsync(2) is a somewhat expensive operation that flushes writes to the
underlying disk/SSD. It's typically used by databases to ensure that
writes survive very hard failure scenarios like your cat kicking the
plug out of the wall. Synchronizing isn't automatically done by
`flush`ing (from the `std::io::Write` or `tokio::io::AsyncWriteExt`
traits). From the [`tokio::fs::File`] moduledocs:

> To ensure that a file is closed immediately when it is dropped, you
> should call `flush` before dropping it. Note that this does not ensure
> that the file has been fully written to disk; the operating system
> might keep the changes around in an in-memory buffer. See the
> `sync_all` method for telling the OS to write the data to disk.

[`tokio::fs::File`]: https://docs.rs/tokio/latest/tokio/fs/struct.File.html
@the-mikedavis
Copy link
Member Author

@EriKWDev how has this patch been faring for you? I'd be thrilled to hear if you haven't been seeing empty files after your computer dies

smortime pushed a commit to smortime/helix that referenced this pull request Jul 10, 2024
fsync(2) is a somewhat expensive operation that flushes writes to the
underlying disk/SSD. It's typically used by databases to ensure that
writes survive very hard failure scenarios like your cat kicking the
plug out of the wall. Synchronizing isn't automatically done by
`flush`ing (from the `std::io::Write` or `tokio::io::AsyncWriteExt`
traits). From the [`tokio::fs::File`] moduledocs:

> To ensure that a file is closed immediately when it is dropped, you
> should call `flush` before dropping it. Note that this does not ensure
> that the file has been fully written to disk; the operating system
> might keep the changes around in an in-memory buffer. See the
> `sync_all` method for telling the OS to write the data to disk.

[`tokio::fs::File`]: https://docs.rs/tokio/latest/tokio/fs/struct.File.html
@Rudxain
Copy link
Contributor

Rudxain commented Jul 16, 2024

I understand that data preservation is more important than performance, but wouldn't it be better if the sync happened on quit (:q)? This amortizes SSD write-cycles (better lifetime) while providing enough data preservation

@the-mikedavis
Copy link
Member Author

I wouldn't consider safety-on-quit to be enough. Some people leave the editor open for long stretches and unexpected system failures come at any time. I'm also not convinced this fsyncing would have much impact on an SSD's lifetime. Compared to something like how a database uses fsync, an fsync per :write is not much at all. Plus this behavior matches (neo)vim

@Rudxain
Copy link
Contributor

Rudxain commented Jul 16, 2024

Some people leave the editor open for long stretches

True. That usually happens to me when I'm "multitasking" (I'm bad at multitasking 😅)

@Rope-a-dope
Copy link

So does backup work now? I fount Helix two days ago and I found the problem. With NeoVim, you can always undo if you open the file again, but you can't do that with Helix.

@kirawi
Copy link
Member

kirawi commented Nov 4, 2024

No, that depends on #9154

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants